Skip to content

Fix RandomSampleImputer and DropMissingData on non-pandas backends - #1024

Merged
solegalli merged 1 commit into
narwhals-migrationfrom
narwhals-fix-random-sample-and-drop-missing
Aug 30, 2026
Merged

solegalli merged 1 commit into
narwhals-migrationfrom
narwhals-fix-random-sample-and-drop-missing

Conversation

@solegalli

Copy link
Copy Markdown
Collaborator

RandomSampleImputer._transform_narwhals had two bugs on the polars/narwhals path:

  • it wrote each variable's imputation into a fresh copy of the input (nw_X = X.with_columns(...)), so only the last imputed variable survived and every earlier variable kept its nulls; it also returned a narwhals frame instead of a native one.
  • the "observation" seed branch read nw_X before it was ever assigned, raising UnboundLocalError. Both branches now accumulate into X and the method returns X.to_native(), matching the pandas branch.

TransformXyMixin.transform_x_y still assumed check_X_y returned a native dataframe. Since check_X_y now returns a narwhals frame, is_pandas_dataframe was always False, so pandas input took the positional-backend path and the __feature_engine_row_index__ tag column made transform() fail the column-count check. The mixin now branches on implementation.is_pandas(), and _check_X_matches_training_df ignores the reserved tag column (its name is now a shared constant in dataframe_checks).

Fixes the polars cases of test_random_sample_imputer.py and both cases of test_drop_missing_data.py::test_transform_x_y.

@solegalli
solegalli force-pushed the narwhals-fix-random-sample-and-drop-missing branch 2 times, most recently from bb89371 to b58be49 Compare August 30, 2026 18:20
RandomSampleImputer._transform_narwhals had two bugs on the polars/narwhals
path:
- it wrote each variable's imputation into a fresh copy of the input
  (`nw_X = X.with_columns(...)`), so only the last imputed variable survived
  and every earlier variable kept its nulls; it also returned a narwhals
  frame instead of a native one.
- the "observation" seed branch read `nw_X` before it was ever assigned,
  raising UnboundLocalError.
Both branches now accumulate into `X` and the method returns `X.to_native()`,
matching the pandas branch.

TransformXyMixin.transform_x_y still assumed check_X_y returned a native
dataframe. Since check_X_y now returns a narwhals frame, `is_pandas_dataframe`
was always False, so pandas input took the positional-backend path and the
`__feature_engine_row_index__` tag column made transform() fail the
column-count check. The mixin now branches on `implementation.is_pandas()`,
and `_check_X_matches_training_df` ignores the reserved tag column (its name
is now a shared constant in dataframe_checks).

Fixes the polars cases of test_random_sample_imputer.py and both cases of
test_drop_missing_data.py::test_transform_x_y.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@solegalli
solegalli force-pushed the narwhals-fix-random-sample-and-drop-missing branch from b58be49 to a6ac9dd Compare August 30, 2026 18:23
@solegalli
solegalli merged commit 6def4de into narwhals-migration Aug 30, 2026
4 of 10 checks passed
@solegalli
solegalli deleted the narwhals-fix-random-sample-and-drop-missing branch August 30, 2026 18:38
solegalli added a commit that referenced this pull request Sep 14, 2026
_check_transform_input_and_state() now returns the narwhals frame, so
filter it directly instead of re-wrapping it with nw.from_native().

TransformXyMixin.transform_x_y() (rebased onto #1024) widens
n_features_in_ for the row-index tag column; also widen
feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and
would otherwise drop the tag column on non-pandas backends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli added a commit that referenced this pull request Sep 15, 2026
_check_transform_input_and_state() now returns the narwhals frame, so
filter it directly instead of re-wrapping it with nw.from_native().

TransformXyMixin.transform_x_y() (rebased onto #1024) widens
n_features_in_ for the row-index tag column; also widen
feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and
would otherwise drop the tag column on non-pandas backends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli added a commit that referenced this pull request Sep 19, 2026
_check_transform_input_and_state() now returns the narwhals frame, so
filter it directly instead of re-wrapping it with nw.from_native().

TransformXyMixin.transform_x_y() (rebased onto #1024) widens
n_features_in_ for the row-index tag column; also widen
feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and
would otherwise drop the tag column on non-pandas backends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli added a commit that referenced this pull request Sep 19, 2026
_check_transform_input_and_state() now returns the narwhals frame, so
filter it directly instead of re-wrapping it with nw.from_native().

TransformXyMixin.transform_x_y() (rebased onto #1024) widens
n_features_in_ for the row-index tag column; also widen
feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and
would otherwise drop the tag column on non-pandas backends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli added a commit that referenced this pull request Sep 19, 2026
_check_transform_input_and_state() now returns the narwhals frame, so
filter it directly instead of re-wrapping it with nw.from_native().

TransformXyMixin.transform_x_y() (rebased onto #1024) widens
n_features_in_ for the row-index tag column; also widen
feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and
would otherwise drop the tag column on non-pandas backends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli added a commit that referenced this pull request Sep 19, 2026
* Migrate OutlierTrimmer to narwhals, add polars support

transform() now filters rows via a single narwhals .filter() call built
from a combined boolean expression (AND of each variable's right/left
cap conditions), instead of a pandas .loc masking loop. Benchmarked
against pandas-native and a numpy boolean-mask extraction at 10k/50k/
100k rows x 1/2/10 columns: the narwhals filter is within 1.4-1.75x of
pandas-native at 10k rows (sub-millisecond absolute difference) and
becomes faster than pandas-native from 50k rows up (0.58x-0.97x),
so a single merged code path (no is_pandas branching) is the right
call here - unlike BaseOutlier's elementwise capping, which benefits
from numpy grouping, row-filtering is exactly what narwhals .filter()
already pushes down to the native backend efficiently.

Also fixes a latent bug in TransformXyMixin.transform_x_y()
(_base_transformers/mixins.py): the non-pandas branch added a
row-index marker column via with_row_index() and passed it straight
to self.transform(), but never widened feature_names_in_/
n_features_in_ to account for it. Any transform() that validates
column count (BaseOutlier._check_transform_input_and_state, via
_check_X_matches_training_df) then raised a ValueError on the extra
column. This was latent because no narwhals-migrated class on this
branch previously combined TransformXyMixin with a column-count-
checking transform() on a non-pandas backend - OutlierTrimmer is the
first. The fix (guarded widen/restore of feature_names_in_ around the
transform() call) is carried over verbatim from the same fix already
applied to this file on branch narwhals-drop-missing-data (commit
fd99caf), which hadn't been merged into this branch yet.

Tests rewritten to one parametrized test per behavior over
make_df in [pd.DataFrame, pl.DataFrame], plus a new test asserting
that caps on two different variables combine with AND (each variable
drops a distinct row) - a code path the old sequential-loop version
exercised implicitly but no test isolated directly.

Docs verified against live output: the class docstring's pandas
examples were already accurate; the user guide's Titanic-based
numbers had drifted from the current openml dataset (predates this
migration, e.g. the IQR section's age max was already wrong against
the old pandas-loop transform()) and are corrected here, plus a "With
polars" section is added.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Adapt OutlierTrimmer to narwhals-returning check_X

_check_transform_input_and_state() now returns the narwhals frame, so
filter it directly instead of re-wrapping it with nw.from_native().

TransformXyMixin.transform_x_y() (rebased onto #1024) widens
n_features_in_ for the row-index tag column; also widen
feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and
would otherwise drop the tag column on non-pandas backends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Use shared backend test fixtures and helpers in OutlierTrimmer tests

Replace the file-local data dicts and _cols/_to_list/_make_series helpers
with the shared test structure: make_df and data_normal_dist / data_na
fixtures, y built with make_series, isinstance(X, make_df) plus to_dict()
checks, and pytest.raises(match=...).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Use frame_to_dict after the shared helper rename in #1045

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Document and test infinite caps for variables without variation

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Align OutlierTrimmer and its tests with the repo conventions

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Keep transform_x_y row alignment in OutlierTrimmer instead of the shared mixin

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant